Skip to content

feat: validate user input values the same way as "default" #381

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Draft
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

Emyrk
Copy link
Member

@Emyrk Emyrk commented Apr 14, 2025

Some highlights of the previous behavior:

  • Validation logic does not apply to option values. So you can have invalid options being displayed. Selecting them would fail at apply
  • Validation logic does not apply to the default value iff an input value is given
    • This is ok at runtime since the value overwrites the default, but at template import time, this kind of error would probably be good to raise. As if the value is removed, then the default is invalid.
  • The input value does not need to be in the option set. This is only enforced in coder/coder
  • Number params do not require the input to be a number. The input value can be "foo" for example.
    • Same with list(string) and probably bool.
  • Empty values are accepted unless a validation block is present.

@Emyrk
Copy link
Member Author

Emyrk commented Apr 14, 2025

@johnstcn @matifali should this pass a template import?

Before this PR, this would pass a terraform plan.

data "coder_parameter" "region" {
  name        = "region"
  description = "Which region would you like to deploy to?"
  type        = "string"
  order       = 1

  option {
    name  = "Europe"
    value = "eu"
  }
  option {
    name  = "United States"
    value = "us"
  }
}

EDIT: Test case here

| EmtyOpts | string,number | | | 1,2,3 | | | "" | false | |

This PR sets the error to:

Planning failed. Terraform encountered an error while generating this plan.

╷
│ Error: Value must be a valid option. The value is empty, did you forget to set it with a default or from user input?
│ 
│   with data.coder_parameter.region,
│   on main.tf line 21, in data "coder_parameter" "region":
│   21: data "coder_parameter" "region" {
│ 
│ the value "" must be defined as one of options

To fix this, either the value must be passed via an env var:

CODER_PARAMETER_c697d2981bf416569a16cfbcdec1542b5398f3cc77d2b905819aa99c46ecf6f6=us terraform plan

Or with a default:

data "coder_parameter" "region" {
  # ...
  default = "us"
  # ...
}

I feel like we should reject it? But that is a breaking change.

@matifali
Copy link
Member

matifali commented Apr 14, 2025

There are use cases where an admin may not want to set a default to force the user to choose an option. At least it was possible before this PR. I am not sure how many users were actually using it.

But I have seen requests to support coder_parameter without a default value better: #144

@johnstcn
Copy link
Member

johnstcn commented Apr 15, 2025

There are use cases where an admin may not want to set a default to force the user to choose an option.

This behaviour is problematic because you could end up with a plan that does not correspond to the apply, depending on how the value of the parameter is used.

Would marking the parameter as ephemeral be a possible workaround here?

EDIT: marking a parameter as ephemeral requires it to be mutable and to also have a default set. This may not suit certain use-cases.

@matifali
Copy link
Member

matifali commented Apr 15, 2025

Do we require a user input if the parameters are ephemeral? Do we not allow setting a default value for such parameters?
Do such ephemeral parameters support options too?

If the answers are yes, then we just need better docs on how to fulfill this use case.

So that people don't try to use the existing non ephemeral parameters.

@Emyrk
Copy link
Member Author

Emyrk commented Apr 29, 2025

@matifali @johnstcn

How I see it is the relaxed validation on template import is incorrect. If a parameter has 0 values, but will have some value at workspace create, then the terraform plan could be incorrect.

However, I understand the use case. The provider SDK we are using does not indicate anywhere if terraform plan vs terraform apply is being run. So there is no way to conditionally enforce validation that way.

My gut thought is that we need to effectively have 2 modes of validation. 1 for template import, 1 for workspace create, where the latter is strict. The strict validation is the most correct.

What if we pass through an env var a relaxed validation mode? coder/coder can pass CODER_VALIDATION=relaxed when doing a template import. Then the provider can accept null values.

@johnstcn
Copy link
Member

What if we pass through an env var a relaxed validation mode? coder/coder can pass CODER_VALIDATION=relaxed when doing a template import. Then the provider can accept null values.

I think this approach could work, but we'd want to perhaps tie its behaviour specifically to what "operation" we're doing in Coder. We might also want to be wary of folks just setting this env var on the provisioner to work around null values entirely.

@Emyrk
Copy link
Member Author

Emyrk commented Apr 30, 2025

I think this approach could work, but we'd want to perhaps tie its behaviour specifically to what "operation" we're doing in Coder. We might also want to be wary of folks just setting this env var on the provisioner to work around null values entirely.

Yes, template import would have to pass some env var to change the behavior.

As for folks setting this env var at home, I think that would just be on them. By default the validation should be handled correctly in coder (when to be relaxed, and when not)

@Emyrk Emyrk force-pushed the stevenmasley/value_validate branch from a61664c to 5e19e2a Compare April 30, 2025 20:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants